Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit Test for FAR Controller #11

Merged
merged 10 commits into from
Feb 28, 2023

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Feb 16, 2023

  • Create new controllers suite for testing with Ginkgo commands.
  • Add Ginkgo target.
  • Refactor Executer
  • Unit Test the build of CLI command and functionality tests.

@razo7 razo7 mentioned this pull request Feb 16, 2023
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So.... I left "some" comments already. But before even understanding the actual test, I discovered a huge mistake in this operator. Looking further into the unit test does not make sense with it:

The FenceAgentsRemediationTemplate's spec should only have a template.spec field which refers to the spec of FenceAgentsRemediation. Everything else needs to go into the latter. See SNR template as an example.

main.go Outdated Show resolved Hide resolved
pkg/cli/cliexecuter.go Outdated Show resolved Hide resolved
controllers/controllers_suite_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller_test.go Outdated Show resolved Hide resolved
controllers/fenceagentsremediation_controller.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@mshitrit
Copy link
Member

mshitrit commented Feb 19, 2023

The FenceAgentsRemediationTemplate's spec should only have a template.spec field which refers to the spec of FenceAgentsRemediation. Everything else needs to go into the latter. See SNR template as an example.

@slintes IIUC this is a mistake because that makes the naming/convention very confusing (i.e the FAR isn't an imaged of the FAR Template).
Is that indeed the case or were you thinking of different reasons ?

ex, err := cli.NewExecuter(pod)
//TODO: Check that FA is excutable? run cli.IsExecuteable

r.Log.Info("Create and execute the fence agent", "Fence Agent", farTemplate.Spec.Agent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: IMO this log is an overkill for each time reconcile is triggered.
(Keep in mind that we already have a log in the executor that has this info an more)

Using Ginkgo commands to bootsrap a new suite - controllers_test
Add Ginkgo target and run it with extra flags instead of go test, since Ginkgo has additional capabilities that can only be accessed via ginkgo
Simplify the code rather than using an hook function
New suite and mockExcuter to mock production code using Executor
make test failed on Github CI
Previously it has been stored in a global variable
The code is better readable
@razo7 razo7 force-pushed the unit-test-far-controller branch 2 times, most recently from 9fb7929 to da25926 Compare February 28, 2023 09:10
Better testing of buildFenceAgentParams result
Better testing of a real agent or a non empty string
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding this test resulted in much better code in the end. Also the test itself evolved nice over time. Good work!

@openshift-ci openshift-ci bot added the lgtm label Feb 28, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7, slintes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c6942e7 into medik8s:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants